Revert "fs,win: fix bug in paths with trailing slashes" #55527

rvagg · 2024-10-25T02:30:10Z

This reverts commit 00b2f07.

Reverts: #54160
Unfixes: #17801

Two problems with #54160 from my experience with things failing on v23:

It assumes that you shouldn't be able to open(2) a directory and acts as if O_DIRECTORY has been supplied, which it hasn't. It's always been possible to fs.open() a directory and use the file descriptor for it. If I happen to pass a trailing slash it shouldn't matter.
It throws, even if I supply a callback.

Example of code broken on v23 that's worked up until now:

const fs = require('fs')

fs.rmdir('_test', () => {
  fs.mkdir('_test', (err) => {
    if (err) return console.error(err)

    try {
      fs.open('_test/', 'r', (err, fd) => {
        if (err) return console.error(err)

        fs.fstat(fd, (err, stat) => {
          if (err) return console.error(err)

          console.log('isDirectory:', stat.isDirectory())
        })
      })
    } catch (err) {
      console.error('Why am I catching an error when I provided a callback?')
      console.error(err)
    }
  })
})

/cc @huseyinacacak-janea @mcollina @anonrig

This reverts commit 00b2f07.

Ref: nodejs/node#55527

codecov · 2024-10-25T03:46:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.42%. Comparing base (53b1050) to head (98bfc2e).
Report is 57 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55527      +/-   ##
==========================================
- Coverage   88.42%   88.42%   -0.01%     
==========================================
  Files         654      654              
  Lines      187552   187509      -43     
  Branches    36087    36075      -12     
==========================================
- Hits       165839   165800      -39     
+ Misses      14950    14947       -3     
+ Partials     6763     6762       -1

Files with missing lines	Coverage Δ
lib/fs.js	`98.14% <100.00%> (+0.02%)`	⬆️
lib/internal/fs/promises.js	`98.24% <100.00%> (ø)`
lib/internal/fs/streams.js	`93.63% <100.00%> (ø)`
lib/internal/fs/utils.js	`99.69% <100.00%> (-0.02%)`	⬇️

... and 26 files with indirect coverage changes

Ref: nodejs/node#55527

huseyinacacak-janea · 2024-10-25T09:22:15Z

From what I've seen, the problem is in open and openSync. Instead of reverting the whole PR, we can just fix the issue in these functions by opening a new PR.
For example, below patch can fix this issue in open and openSync. There might be different fixes but I'm sharing this one to show my approach.

patch

diff --git a/lib/fs.js b/lib/fs.js
index e6ae9855a0d..6207bef8c05 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -537,7 +537,8 @@ function closeSync(fd) {
* @returns {void}
*/
function open(path, flags, mode, callback) {
-  path = getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' });
+  const isDirectory = lstatSync(path, {throwIfNoEntry: false})?.isDirectory();
+  path = getValidatedPath(path, 'path', { expectFile: !isDirectory, syscall: 'open' });
 if (arguments.length < 3) {
   callback = flags;
   flags = 'r';
@@ -565,8 +566,9 @@ function open(path, flags, mode, callback) {
* @returns {number}
*/
function openSync(path, flags, mode) {
+  const isDirectory = lstatSync(path, {throwIfNoEntry: false})?.isDirectory();
 return binding.open(
-    getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' }),
+    getValidatedPath(path, 'path', { expectFile: !isDirectory, syscall: 'open' }),
   stringToFlags(flags),
   parseFileMode(mode, 'mode', 0o666),
 );
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index f5a9a3a8542..f029d28d752 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -633,7 +633,8 @@ async function copyFile(src, dest, mode) {
// Note that unlike fs.open() which uses numeric file descriptors,
// fsPromises.open() uses the fs.FileHandle class.
async function open(path, flags, mode) {
-  path = getValidatedPath(path, 'path', { expectFile: true, syscall: 'open' });
+  const stats = await lstat(path);
+  path = getValidatedPath(path, 'path', { expectFile: !stats.isDirectory(), syscall: 'open' });
 const flagsNumber = stringToFlags(flags);
 mode = parseFileMode(mode, 'mode', 0o666);
 return new FileHandle(await PromisePrototypeThen(

What do you think?

rvagg · 2024-10-25T10:14:12Z

I don't really like that you're doing a stat before I get to open the file descriptor, part of the reason one might deal directly with file descriptors is to be able to manage the lifecycle much more carefully. In the particular use-case where this error came up for me I'm doing precisely this - open fd, do a stat on it, deal with it in a particular way if it's a directory or not, but hold on to the fd for further operations later; a very fine-grained handling of the filesystem in a particularly i/o heavy situation. Your solution now wants to essentially load my file descriptor twice before I can get my hands on it.

I'd also like errors to be dealt with via the callback if one is present, passing that down into getValidatedPath would seem appropriate so it can figure out whether to throw or return.

blexrob · 2024-10-30T12:06:59Z

This will probably also fix emscripten-core/emscripten#22812

RafaelGSS · 2024-10-30T18:18:05Z

@rvagg However, landing this would result in another semver-major change I suppose.

ruyadorno · 2024-10-30T18:42:37Z

@rvagg However, landing this would result in another semver-major change I suppose.

well, assuming this was an unintentional API change, it should be just a patch bugfix

rvagg · 2024-10-30T23:58:51Z

I think it would have to be treated as a regression and therefore a semver-patch. It's unfortunate because the original fix was trying to address a problem and this undoes that. But maybe there's an alternative path to addressing that problem that doesn't introduce a regression could be bundled along with a revert of the initial attempt.

rvagg · 2024-10-30T23:59:27Z

I note esbuild has also identified this as a problem.

juanarbol · 2024-10-31T04:57:22Z

I note esbuild has also identified this as a problem.

Same for webpack webpack/webpack#18891

This will fix #55609

juanarbol

LGTM

nodejs-github-bot · 2024-10-31T13:45:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63362/

nodejs-github-bot · 2024-10-31T16:01:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63364/

rvagg · 2024-11-01T00:46:09Z

Is the TLS test failure a known flaky?

---
duration_ms: 688.045
exitcode: 1
severity: fail
stack: |-
  node:events:485
        throw er; // Unhandled 'error' event
        ^

  Error: E4080000:error:0A00041A:SSL routines:ssl3_read_bytes:tlsv1 alert decode error:C:\workspace\node-compile-windows\node\deps\openssl\openssl\ssl\record\rec_layer_s3.c:1605:SSL alert number 50

  Emitted 'error' event on TLSSocket instance at:
      at TLSSocket._emitTLSError (node:_tls_wrap:1034:10)
      at TLSWrap.onerror (node:_tls_wrap:475:11) {
    library: 'SSL routines',
    reason: 'tlsv1 alert decode error',
    code: 'ERR_SSL_TLSV1_ALERT_DECODE_ERROR'
  }

  Node.js v24.0.0-pre
...

nodejs-github-bot · 2024-11-01T06:34:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63374/

mcollina

lgtm

nodejs-github-bot · 2024-11-02T16:24:30Z

Landed in 7b01758

This reverts commit 00b2f07. PR-URL: nodejs#55527 Fixes: nodejs#17801 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>

This reverts commit 00b2f07. PR-URL: #55527 Fixes: #17801 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>

This reverts commit 00b2f07. PR-URL: nodejs#55527 Fixes: nodejs#17801 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>

…6417 Looks like a nodejs bug, introduced in v23.0.0 - commit hash 00b2f07f It should be fixed in v23.2.0 - dd9b6833 bug: nodejs/node#54160 fix: nodejs/node#55527 I am able to replicate this on v23.1.0.

Revert "fs,win: fix bug in paths with trailing slashes"

98bfc2e

This reverts commit 00b2f07.

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Oct 25, 2024

rvagg added a commit to isaacs/st that referenced this pull request Oct 25, 2024

fix: remove trailing slashes for fs.open to address Node v23 bug

588067f

Ref: nodejs/node#55527

rvagg mentioned this pull request Oct 25, 2024

fix: remove trailing slashes for fs.open to address Node v23 bug isaacs/st#98

Merged

rvagg added a commit to isaacs/st that referenced this pull request Oct 25, 2024

fix: remove trailing slashes for fs.open to address Node v23 bug

059bd82

Ref: nodejs/node#55527

rvagg added a commit to isaacs/st that referenced this pull request Oct 25, 2024

fix: remove trailing slashes for fs.open to address Node v23 bug

4ff6f98

Ref: nodejs/node#55527

merceyz mentioned this pull request Oct 25, 2024

esbuild-wasm hangs on Node 23 evanw/esbuild#3951

Closed

blexrob mentioned this pull request Oct 30, 2024

opendir doesn't work on node 23 for paths ending in a slash emscripten-core/emscripten#22812

Open

RafaelGSS approved these changes Oct 30, 2024

View reviewed changes

RafaelGSS mentioned this pull request Oct 30, 2024

Enforce a more strict policy for semver-major releases nodejs/Release#1054

Closed

juanarbol approved these changes Oct 31, 2024

View reviewed changes

marco-ippolito approved these changes Oct 31, 2024

View reviewed changes

lpinca approved these changes Oct 31, 2024

View reviewed changes

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2024

avivkeller added the revert PRs that revert previously landed PRs. label Oct 31, 2024

mcollina approved these changes Nov 1, 2024

View reviewed changes

StefanStojanovic approved these changes Nov 1, 2024

View reviewed changes

marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 2, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 2, 2024

nodejs-github-bot merged commit 7b01758 into nodejs:main Nov 2, 2024
84 checks passed

StefanStojanovic mentioned this pull request Nov 5, 2024

Inconsistent behavior with fs module accross platforms #17801

Closed

aduh95 mentioned this pull request Nov 6, 2024

2024-11-11, Version 23.2.0 (Current) #55741

Merged

connor4312 mentioned this pull request Nov 22, 2024

Node.js v23.2.0: Breakpoints not binding in VS Code debugger #55948

Closed

jeremybobbin mentioned this pull request Dec 5, 2024

prevent reading directories as files when building fs cache #16417 webpack/webpack#19036

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "fs,win: fix bug in paths with trailing slashes" #55527

Revert "fs,win: fix bug in paths with trailing slashes" #55527

rvagg commented Oct 25, 2024

codecov bot commented Oct 25, 2024 •

edited

Loading

huseyinacacak-janea commented Oct 25, 2024

rvagg commented Oct 25, 2024

blexrob commented Oct 30, 2024

RafaelGSS commented Oct 30, 2024

ruyadorno commented Oct 30, 2024

rvagg commented Oct 30, 2024

rvagg commented Oct 30, 2024

juanarbol commented Oct 31, 2024 •

edited

Loading

juanarbol left a comment

nodejs-github-bot commented Oct 31, 2024

nodejs-github-bot commented Oct 31, 2024

rvagg commented Nov 1, 2024

nodejs-github-bot commented Nov 1, 2024

mcollina left a comment

nodejs-github-bot commented Nov 2, 2024

Revert "fs,win: fix bug in paths with trailing slashes" #55527

Revert "fs,win: fix bug in paths with trailing slashes" #55527

Conversation

rvagg commented Oct 25, 2024

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

huseyinacacak-janea commented Oct 25, 2024

rvagg commented Oct 25, 2024

blexrob commented Oct 30, 2024

RafaelGSS commented Oct 30, 2024

ruyadorno commented Oct 30, 2024

rvagg commented Oct 30, 2024

rvagg commented Oct 30, 2024

juanarbol commented Oct 31, 2024 • edited Loading

juanarbol left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 31, 2024

nodejs-github-bot commented Oct 31, 2024

rvagg commented Nov 1, 2024

nodejs-github-bot commented Nov 1, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 2, 2024

codecov bot commented Oct 25, 2024 •

edited

Loading

juanarbol commented Oct 31, 2024 •

edited

Loading